Ensure no nodes can depend on themselves even when transcoding is used #3812
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ivan Danov <[email protected]>
Signed-off-by: Ivan Danov <[email protected]>
merelcht
approved these changes
Apr 15, 2024
There was a problem hiding this comment.
Thanks for fixing this @idanov and of course @ElenaKhaustova and @noklam for investigating beforehand ⭐
I think it's a good idea to add the transcoding check as part of the node validation. Also a good call on moving the transcoding related methods into a separate file 👍
7 tasks
Signed-off-by: Elena Khaustova <[email protected]>
Signed-off-by: Elena Khaustova <[email protected]>
ElenaKhaustova
approved these changes
Apr 15, 2024
|
This accidentally made a small breakage of backwards incompatibility kedro-org/kedro-viz#1865 |
Shall we somehow highlight this in the current release 0.19.4? |
AhdraMeraliQB
pushed a commit
that referenced
this pull request
Apr 17, 2024
#3812) * Factor out transcoding helpers into a private module Signed-off-by: Ivan Danov <[email protected]> * Ensure node input/output validation doesn't allow transcoded self-loops Signed-off-by: Ivan Danov <[email protected]> * Updated release note to avoid github warning Signed-off-by: Elena Khaustova <[email protected]> --------- Signed-off-by: Ivan Danov <[email protected]> Signed-off-by: Elena Khaustova <[email protected]> Co-authored-by: Elena Khaustova <[email protected]> Signed-off-by: Ahdra Merali <[email protected]>
AhdraMeraliQB
added a commit
that referenced
this pull request
Apr 17, 2024
) * Update kedro-catalog-0.19.json (#3724) * Update kedro-catalog-0.19.json Signed-off-by: Anthony De Bortoli <[email protected]> * Update set_up_vscode.md Signed-off-by: Anthony De Bortoli <[email protected]> --------- Signed-off-by: Anthony De Bortoli <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Update project tests directory structure in docs Signed-off-by: Ahdra Merali <[email protected]> * Add docs on writing tests Signed-off-by: Ahdra Merali <[email protected]> * Drop dependency on toposort in favour of built-in graphlib (#3728) * Replace toposort with graphlib (built-in from Python 3.9) Signed-off-by: Ivan Danov <[email protected]> * Create toposort groups only when needed Signed-off-by: Ivan Danov <[email protected]> * Update RELEASE.md and graphlib version constraints Signed-off-by: Ivan Danov <[email protected]> * Remove mypy-toposort Signed-off-by: Ivan Danov <[email protected]> * Ensure that the suggest resume test has no node ordering requirement Signed-off-by: Ivan Danov <[email protected]> * Ensure stable toposorting by grouping and ungrouping the result Signed-off-by: Ivan Danov <[email protected]> --------- Signed-off-by: Ivan Danov <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Optimise pipeline addition and creation (#3730) * Create toposort groups only when needed * Ensure that the suggest resume test has no node ordering requirement * Ensure stable toposorting by grouping and ungrouping the result * Delay toposorting until pipeline.nodes is used * Avoid using .nodes when topological order or new copy is unneeded * Copy the nodes only if tags are provided * Remove unnecessary condition in self.nodes Signed-off-by: Ivan Danov <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Expand robots.txt for Kedro-Viz and Kedro-Datasets docs (#3729) * Add project to robots.txt Signed-off-by: Dmitry Sorokin <[email protected]> * Add EOF Signed-off-by: Dmitry Sorokin <[email protected]> --------- Signed-off-by: Dmitry Sorokin <[email protected]> Co-authored-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Kedro need more uv (#3740) * Kedro need more uv Signed-off-by: Nok <[email protected]> * remove docker Signed-off-by: Nok <[email protected]> --------- Signed-off-by: Nok <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Resolve all path in Kedro (#3742) * Kedro need more uv Signed-off-by: Nok <[email protected]> * remove docker Signed-off-by: Nok <[email protected]> * fix broken type hint and resolve project path Signed-off-by: Nok Lam Chan <[email protected]> * fix type hint Signed-off-by: Nok Lam Chan <[email protected]> * remove duplicate logic Signed-off-by: Nok Lam Chan <[email protected]> * adding nok.py is definitely an accident Signed-off-by: Nok Lam Chan <[email protected]> * fix test Signed-off-by: Nok Lam Chan <[email protected]> * remove print Signed-off-by: Nok Lam Chan <[email protected]> * add test Signed-off-by: Nok <[email protected]> --------- Signed-off-by: Nok <[email protected]> Signed-off-by: Nok Lam Chan <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Remove settings of rate limits and retries (#3769) * double linkcheck limits Signed-off-by: Nok Lam Chan <[email protected]> * fix ratelimit Signed-off-by: Nok <[email protected]> --------- Signed-off-by: Nok Lam Chan <[email protected]> Signed-off-by: Nok <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Improve resume suggestions (#3719) * Improve suggestions to resume a failed pipeline - if dataset (or param) is persistent & shared, don't keep looking for ancestors - only look for ancestors producing impersistent inputs - minimize number of suggested nodes (= shorter message for the same pipeline) - testable logic, tests cases outside of scenarios for sequential runner - Use _EPHEMERAL attribute - Move tests to separate file - Docstring updates --------- Signed-off-by: Ondrej Zacha <[email protected]> Co-authored-by: Nok Lam Chan <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Build docs fix (#3773) * Ignored forbidden url Signed-off-by: Elena Khaustova <[email protected]> * Returned linkscheck retries Signed-off-by: Elena Khaustova <[email protected]> * Removed odd comment Signed-off-by: Elena Khaustova <[email protected]> --------- Signed-off-by: Elena Khaustova <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Clarify docs around custom resolvers (#3759) * Updated custom resolver docs section Signed-off-by: Elena Khaustova <[email protected]> * Updated advanced configuration section for consistency Signed-off-by: Elena Khaustova <[email protected]> * Updated RELEASE.md Signed-off-by: Elena Khaustova <[email protected]> * Updated RELEASE.md Signed-off-by: Elena Khaustova <[email protected]> * Test linkcheck_workers decrease Signed-off-by: Elena Khaustova <[email protected]> * Increased the By default, the linkcheck_rate_limit_timeout to default Signed-off-by: Elena Khaustova <[email protected]> * Returned old docs build settings Signed-off-by: Elena Khaustova <[email protected]> * Fixed typo Signed-off-by: Elena Khaustova <[email protected]> * Ignore forbidden url Signed-off-by: Elena Khaustova <[email protected]> * Returned linkcheck retries Signed-off-by: Elena Khaustova <[email protected]> --------- Signed-off-by: Elena Khaustova <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Add mlruns to gitignore to avoid pushing mlflow local runs to github (#3765) * Add mlruns to gitignore to avoid pushing mlflow local runs to github Signed-off-by: Yolan Honoré-Rougé <[email protected]> * update release.md Signed-off-by: Yolan Honoré-Rougé <[email protected]> --------- Signed-off-by: Yolan Honoré-Rougé <[email protected]> Signed-off-by: Merel Theisen <[email protected]> Co-authored-by: Merel Theisen <[email protected]> Co-authored-by: Nok Lam Chan <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Update the dependencies page in the docs (#3772) * Update the dependencies page Signed-off-by: Ankita Katiyar <[email protected]> * Update docs/source/kedro_project_setup/dependencies.md Signed-off-by: Ankita Katiyar <[email protected]> * Apply suggestions from code review Co-authored-by: Jo Stichbury <[email protected]> Signed-off-by: Ankita Katiyar <[email protected]> * Fix lint Signed-off-by: Ankita Katiyar <[email protected]> * Move the last line to notes Signed-off-by: Ankita Katiyar <[email protected]> --------- Signed-off-by: Ankita Katiyar <[email protected]> Signed-off-by: Ankita Katiyar <[email protected]> Co-authored-by: Jo Stichbury <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Change pipeline test location to project root/tests (#3731) * Change pipeline test location to project root/tests Signed-off-by: lrcouto <[email protected]> * Fix some test_pipeline tests Signed-off-by: lrcouto <[email protected]> * Change delete pipeline to account for new structure Signed-off-by: lrcouto <[email protected]> * Fix some tests Signed-off-by: lrcouto <[email protected]> * Change tests path on micropkg Signed-off-by: lrcouto <[email protected]> * Fix remaining tests Signed-off-by: lrcouto <[email protected]> * Add changes to release notes Signed-off-by: lrcouto <[email protected]> * Update file structure on micropackaging doc page Signed-off-by: lrcouto <[email protected]> --------- Signed-off-by: lrcouto <[email protected]> Signed-off-by: L. R. Couto <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Add an option for kedro new to skip telemetry (#3701) * First draft for telemetry consent flag on kedro new Signed-off-by: lrcouto <[email protected]> * Add functioning --telemetry option to kedro new Signed-off-by: lrcouto <[email protected]> * Update tests to acknowledge new flag Signed-off-by: lrcouto <[email protected]> * Add tests for kedro new --telemetry flag Signed-off-by: lrcouto <[email protected]> * Add changes to documentation and release notes Signed-off-by: lrcouto <[email protected]> * Minor change to docs Signed-off-by: lrcouto <[email protected]> * Lint Signed-off-by: lrcouto <[email protected]> * Remove outdated comment and correct type hint Signed-off-by: lrcouto <[email protected]> * Update docs/source/get_started/new_project.md Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: L. R. Couto <[email protected]> * Lint Signed-off-by: lrcouto <[email protected]> * Minor change on release note Signed-off-by: lrcouto <[email protected]> --------- Signed-off-by: lrcouto <[email protected]> Signed-off-by: L. R. Couto <[email protected]> Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Update documentation for OmegaConfigLoader (#3778) * Update documentation for OmegaConfigLoader Signed-off-by: Puneet Saini <[email protected]> * Update RELEASE.md Signed-off-by: Puneet Saini <[email protected]> * Update RELEASE.md Signed-off-by: Puneet Saini <[email protected]> * Update ignore-names.txt Signed-off-by: Juan Luis Cano Rodríguez <[email protected]> --------- Signed-off-by: Puneet Saini <[email protected]> Signed-off-by: Juan Luis Cano Rodríguez <[email protected]> Co-authored-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Fix path Signed-off-by: Ahdra Merali <[email protected]> * Lint Signed-off-by: Ahdra Merali <[email protected]> * Add changes to RELEASE.md Signed-off-by: Ahdra Merali <[email protected]> * Address comments from code review Signed-off-by: Ahdra Merali <[email protected]> * Empty Signed-off-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Remove unneeded imports Signed-off-by: Ahdra Merali <[email protected]> * Change recommendation from pytest config to editable install Signed-off-by: Ahdra Merali <[email protected]> * Add negative testing example Signed-off-by: Ahdra Merali <[email protected]> * Replace Dict with dict Signed-off-by: Ahdra Merali <[email protected]> * Remove test classes Signed-off-by: Ahdra Merali <[email protected]> * Change the assert step for the integration test Signed-off-by: Ahdra Merali <[email protected]> * Fix error handling for OmegaConfigLoader (#3784) * Update omegaconf_config.py Signed-off-by: Puneet Saini <[email protected]> * Update RELEASE.md Signed-off-by: Puneet Saini <[email protected]> * add a more complicated test case Signed-off-by: Nok <[email protected]> --------- Signed-off-by: Puneet Saini <[email protected]> Signed-off-by: Nok <[email protected]> Co-authored-by: Nok <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Add Simon Brugman to TSC (#3780) Signed-off-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Update technical_steering_committee.md (#3796) Signed-off-by: Marcin Zabłocki <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Remove jmespath dependency (#3797) Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Update spaceflights tutorial and starter requirements for kedro-datasets optional dependencies (#3664) * Update spaceflights tutorial and starter requirements Signed-off-by: lrcouto <[email protected]> * fix e2e tests Signed-off-by: lrcouto <[email protected]> * Fix e2e tests by distinguishing `kedro-datasets` dependency for different python versions (#3802) Signed-off-by: Merel Theisen <[email protected]> * Update docs/source/tutorial/tutorial_template.md Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: L. R. Couto <[email protected]> --------- Signed-off-by: lrcouto <[email protected]> Signed-off-by: L. R. Couto <[email protected]> Signed-off-by: Merel Theisen <[email protected]> Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Consider Vale's suggestions Signed-off-by: Ahdra Merali <[email protected]> * Hide test in details Signed-off-by: Ahdra Merali <[email protected]> * Quick fix Signed-off-by: Ahdra Merali <[email protected]> * Typo (and wording changes) Signed-off-by: Ahdra Merali <[email protected]> * Update robots.txt (#3803) Signed-off-by: Ankita Katiyar <[email protected]> Co-authored-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Add a test for transcoding loops of 1 or more nodes (#3810) Signed-off-by: Ivan Danov <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Ensure no nodes can depend on themselves even when transcoding is used (#3812) * Factor out transcoding helpers into a private module Signed-off-by: Ivan Danov <[email protected]> * Ensure node input/output validation doesn't allow transcoded self-loops Signed-off-by: Ivan Danov <[email protected]> * Updated release note to avoid github warning Signed-off-by: Elena Khaustova <[email protected]> --------- Signed-off-by: Ivan Danov <[email protected]> Signed-off-by: Elena Khaustova <[email protected]> Co-authored-by: Elena Khaustova <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Update UUID telemetry docs (#3805) Signed-off-by: Dmitry Sorokin <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Change path to starters test (#3816) Signed-off-by: lrcouto <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> * Move changes in RELEASE.md to docs section Signed-off-by: Ahdra Merali <[email protected]> * Change formatting Signed-off-by: Ahdra Merali <[email protected]> * Revert "Change formatting" This reverts commit 9582a22. Signed-off-by: Ahdra Merali <[email protected]> * Apply changes from code review Signed-off-by: Ahdra Merali <[email protected]> * Add explanation on why cleanup isn't needed Signed-off-by: Ahdra Merali <[email protected]> * Change assert on successful pipeline to check logs Signed-off-by: Ahdra Merali <[email protected]> * Update description of integration test under pipeline slicing Signed-off-by: Ahdra Merali <[email protected]> * Missing formatting Signed-off-by: Ahdra Merali <[email protected]> * Update tests directory structure Signed-off-by: Ahdra Merali <[email protected]> --------- Signed-off-by: Anthony De Bortoli <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> Signed-off-by: Ivan Danov <[email protected]> Signed-off-by: Dmitry Sorokin <[email protected]> Signed-off-by: Nok <[email protected]> Signed-off-by: Nok Lam Chan <[email protected]> Signed-off-by: Ondrej Zacha <[email protected]> Signed-off-by: Elena Khaustova <[email protected]> Signed-off-by: Yolan Honoré-Rougé <[email protected]> Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ankita Katiyar <[email protected]> Signed-off-by: Ankita Katiyar <[email protected]> Signed-off-by: lrcouto <[email protected]> Signed-off-by: L. R. Couto <[email protected]> Signed-off-by: Puneet Saini <[email protected]> Signed-off-by: Juan Luis Cano Rodríguez <[email protected]> Signed-off-by: Marcin Zabłocki <[email protected]> Signed-off-by: Merel Theisen <[email protected]> Signed-off-by: Ahdra Merali <[email protected]> Co-authored-by: Anthony De Bortoli <[email protected]> Co-authored-by: Ivan Danov <[email protected]> Co-authored-by: Dmitry Sorokin <[email protected]> Co-authored-by: Juan Luis Cano Rodríguez <[email protected]> Co-authored-by: Nok Lam Chan <[email protected]> Co-authored-by: Ondrej Zacha <[email protected]> Co-authored-by: ElenaKhaustova <[email protected]> Co-authored-by: Yolan Honoré-Rougé <[email protected]> Co-authored-by: Merel Theisen <[email protected]> Co-authored-by: Ankita Katiyar <[email protected]> Co-authored-by: Jo Stichbury <[email protected]> Co-authored-by: L. R. Couto <[email protected]> Co-authored-by: Puneet Saini <[email protected]> Co-authored-by: Marcin Zabłocki <[email protected]> Co-authored-by: Elena Khaustova <[email protected]>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We have figured out in #3799 that Kedro allows self-referencing nodes, as long as they use transcoding in their dataset names as follows:
This makes the transcoding feature inconsistent with basic Kedro invariants like "no loops allowed in the graph". This PR adds a change to the node validation code, which raises an error and makes the message mention transcoded dataset names.
It also adds a couple of tests and moves around a bit some of the transcoding helpers, so they can be accessible to all modules in
kedro.pipeline.While working on this, the PR also reveals a bit of weird place of using
_strip_transcodinginKedroContextwhich should probably be addressed whenKedroSessionis redesigned.Development notes
See above.
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-byline in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.mdfile